Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/save identity provider id #99

Merged
merged 11 commits into from
Jun 25, 2023
Merged

Feature/save identity provider id #99

merged 11 commits into from
Jun 25, 2023

Conversation

lohas1107
Copy link
Contributor

@lohas1107 lohas1107 commented Jun 18, 2023 •
edited by ricksu978
Loading

Why need this change? / Root cause:

  • Change to use JWT token

Changes made:

  • Create user when login first time

Test Scope / Change impact:

  • OAuth2Controller

Issue

@kuoche1712003
Copy link
Contributor

現在登入成功的流程應該會走到 CustomSuccessHandler
可能需要把 28 29 行改一下

...
        val identityProviderId = authentication.principal.let { it as OidcUser }.subject
        createUserUseCase.execute(CreateUserUseCase.Request(identityProviderId))
...

@ricksu978
Copy link
Member

ricksu978 commented Jun 18, 2023 •
edited
Loading

When Creating an User
We also need email field populated.

The created User should look like this:

{
  "id": "{mongoId}",
  "email": "[email protected]",   // <= email 還是要設置
  "nickname": "{randomNickname}",  // <= 記得給 random Nickname, 請看 issue 76
  "identities" : [ "google-oauth2|108242752022034936604" ]
}

Issue #76

@ricksu978 ricksu978 self-requested a review June 18, 2023 13:24
frankvicky
frankvicky requested changes Jun 18, 2023
View reviewed changes
spring/src/main/kotlin/tw/waterballsa/gaas/spring/controllers/OAuth2Controller.kt Outdated
)
private fun Jwt.toRequest(): CreateUserUseCase.Request =
CreateUserUseCase.Request(
this.subject ?: throw PlatformException("JWT subject is null")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this 可以省略

spring/src/main/kotlin/tw/waterballsa/gaas/spring/repositories/SpringUserRepository.kt Outdated
Comment on lines 18 to 19
override fun existsByIdentitiesIn(identityProviderId: String): Boolean =
userDAO.existsByIdentitiesIn(mutableListOf(mutableListOf(identityProviderId)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q：想知道這裡為什麼需要兩層 List？

spring/src/main/kotlin/tw/waterballsa/gaas/spring/repositories/dao/UserDAO.kt Outdated
@@ -7,4 +7,5 @@ import tw.waterballsa.gaas.spring.repositories.data.UserData
@Repository
interface UserDAO : MongoRepository<UserData, String> {
fun existsByEmail(email: String): Boolean
fun existsByIdentitiesIn(identities: MutableCollection<MutableList<String>>): Boolean
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto
Q：想知道這裡為什麼需要兩層 List？

spring/src/main/kotlin/tw/waterballsa/gaas/spring/repositories/data/UserData.kt Outdated
@@ -9,20 +9,23 @@ class UserData(
@Id
var id: String? = null,
private var email: String? = null,
var nickname: String? = null
var nickname: String? = null,
var identities: List<String> = emptyList()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q：這裡有特別的理由需要使用 var 宣告集合嗎？

Wally5077
Wally5077 requested changes Jun 18, 2023
View reviewed changes
Copy link
Collaborator

@Wally5077 Wally5077 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review done.

spring/src/main/kotlin/tw/waterballsa/gaas/spring/controllers/OAuth2Controller.kt Outdated Show resolved Hide resolved
spring/src/main/kotlin/tw/waterballsa/gaas/spring/repositories/SpringUserRepository.kt Outdated Show resolved Hide resolved
spring/src/main/kotlin/tw/waterballsa/gaas/spring/repositories/dao/UserDAO.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/main/kotlin/tw/waterballsa/gaas/spring/repositories/data/UserData.kt Outdated Show resolved Hide resolved
application/src/main/kotlin/tw/waterballsa/gaas/application/repositories/UserRepository.kt Outdated Show resolved Hide resolved
kuoche1712003
kuoche1712003 requested changes Jun 18, 2023
View reviewed changes
Copy link
Contributor

@kuoche1712003 kuoche1712003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review done.

spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
@kuoche1712003
Copy link
Contributor

@Wally5077 @frankvicky
目前登入成功後的邏輯由 CustomSuccessHandler 負責
其實 OAuth2Controller 目前應該是沒有作用的, 需要連同 Test 整個拔掉嗎？
還是其實可以留下來, 因為不知道怎麼測 CustomSuccessHandler

@lohas1107 lohas1107 force-pushed the feature/save-identity-provider-id branch 4 times, most recently from 08a271f to df6f0c2 Compare June 23, 2023 08:42
kuoche1712003
kuoche1712003 requested changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@kuoche1712003 kuoche1712003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review done.

application/src/main/kotlin/tw/waterballsa/gaas/application/usecases/CreateUserUseCase.kt Outdated Show resolved Hide resolved
spring/src/main/kotlin/tw/waterballsa/gaas/spring/configs/securities/CustomSuccessHandler.kt Outdated Show resolved Hide resolved
@lohas1107 lohas1107 force-pushed the feature/save-identity-provider-id branch from df6f0c2 to 744fe74 Compare June 23, 2023 10:26
@lohas1107 lohas1107 requested a review from kuoche1712003 June 23, 2023 10:39
@lohas1107 lohas1107 force-pushed the feature/save-identity-provider-id branch from 744fe74 to b1b970e Compare June 23, 2023 11:57
@lohas1107 lohas1107 requested a review from ricksu978 June 23, 2023 12:06
@lohas1107 lohas1107 force-pushed the feature/save-identity-provider-id branch from a7f2a08 to 75f2fa7 Compare June 23, 2023 12:22
kuoche1712003
kuoche1712003 approved these changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@kuoche1712003 kuoche1712003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

功能部分 ok
code style 交給其他人

Wally5077
Wally5077 requested changes Jun 24, 2023
View reviewed changes
Copy link
Collaborator

@Wally5077 Wally5077 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review done.

domain/src/main/kotlin/tw/waterballsa/gaas/domain/User.kt Outdated Show resolved Hide resolved
domain/src/main/kotlin/tw/waterballsa/gaas/domain/User.kt Outdated Show resolved Hide resolved
application/src/main/kotlin/tw/waterballsa/gaas/application/usecases/CreateUserUseCase.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/AbstractSpringBootTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
@lohas1107 lohas1107 requested a review from Wally5077 June 24, 2023 15:54
@lohas1107 lohas1107 force-pushed the feature/save-identity-provider-id branch from c996e83 to fb8ccd6 Compare June 24, 2023 16:01
@lohas1107 lohas1107 force-pushed the feature/save-identity-provider-id branch from fb8ccd6 to 9affdf0 Compare June 24, 2023 16:44
Wally5077
Wally5077 requested changes Jun 25, 2023
View reviewed changes
Copy link
Collaborator

@Wally5077 Wally5077 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review done.

domain/src/main/kotlin/tw/waterballsa/gaas/domain/User.kt Outdated Show resolved Hide resolved
application/src/main/kotlin/tw/waterballsa/gaas/application/usecases/CreateUserUseCase.kt Outdated Show resolved Hide resolved
application/src/main/kotlin/tw/waterballsa/gaas/application/usecases/CreateUserUseCase.kt Outdated Show resolved Hide resolved
spring/src/main/kotlin/tw/waterballsa/gaas/spring/repositories/data/UserData.kt Outdated Show resolved Hide resolved
spring/src/main/kotlin/tw/waterballsa/gaas/spring/repositories/data/UserData.kt Outdated Show resolved Hide resolved
domain/src/main/kotlin/tw/waterballsa/gaas/domain/User.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/AbstractSpringBootTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
Wally5077
Wally5077 requested changes Jun 25, 2023
View reviewed changes
Copy link
Collaborator

@Wally5077 Wally5077 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review done.

spring/src/main/kotlin/tw/waterballsa/gaas/spring/repositories/data/UserData.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
@lohas1107 lohas1107 requested a review from Wally5077 June 25, 2023 05:34
Wally5077
Wally5077 requested changes Jun 25, 2023
View reviewed changes
Copy link
Collaborator

@Wally5077 Wally5077 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review done.

application/src/main/kotlin/tw/waterballsa/gaas/application/usecases/CreateUserUseCase.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/AbstractSpringBootTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
@lohas1107 lohas1107 requested a review from Wally5077 June 25, 2023 06:13
Wally5077
Wally5077 requested changes Jun 25, 2023
View reviewed changes
Copy link
Collaborator

@Wally5077 Wally5077 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review done.

spring/src/test/kotlin/tw/waterballsa/gaas/spring/it/controllers/OAuth2ControllerTest.kt Outdated Show resolved Hide resolved
Wally5077
Wally5077 approved these changes Jun 25, 2023
View reviewed changes
Copy link
Collaborator

@Wally5077 Wally5077 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lohas1107 lohas1107 merged commit 33a4006 into main Jun 25, 2023
@lohas1107 lohas1107 deleted the feature/save-identity-provider-id branch June 25, 2023 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankvicky frankvicky frankvicky approved these changes

@ricksu978 ricksu978 ricksu978 approved these changes

@kuoche1712003 kuoche1712003 kuoche1712003 approved these changes

@Wally5077 Wally5077 Wally5077 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

登入後，如果沒有暱稱 系統就隨機命名
5 participants
@lohas1107 @kuoche1712003 @ricksu978 @Wally5077 @frankvicky